Skip to content

fix: strip all ANSI escape sequences in terminal command output - #838

Open
OmarAlaaeldein wants to merge 1 commit into
CodebuffAI:mainfrom
OmarAlaaeldein:fix/sudo-ansi-escape-tui-corruption
Open

fix: strip all ANSI escape sequences in terminal command output#838
OmarAlaaeldein wants to merge 1 commit into
CodebuffAI:mainfrom
OmarAlaaeldein:fix/sudo-ansi-escape-tui-corruption

Conversation

@OmarAlaaeldein

@OmarAlaaeldein OmarAlaaeldein commented Jul 3, 2026

Copy link
Copy Markdown

This PR fixes TUI display and scroll corruption caused by VT/ANSI control sequences in captured terminal-command output.

  • Switches the bounded output buffer from stripColors to the existing stripAnsi utility, so cursor movement, alternate-screen, and other control sequences cannot reach the TUI.
  • Leaves child-process TERM and SUDO_PROMPT unchanged.
  • Preserves carriage returns rather than removing them unconditionally.
  • Adds regression coverage for cursor-control sequences.

@codebuff-team

Copy link
Copy Markdown
Contributor

Thanks for digging into the TUI corruption issue - the root cause analysis (control/cursor codes bleeding through stripColors, which only strips SGR color codes) is correct and worth fixing.

A few concerns before this could be ported as-is:

  1. stripAnsi vs stripColors: please confirm stripAnsi actually exists in common/src/util/string (or add it) - the diff only shows the import changing, not the underlying utility being added/verified.

  2. Forcing TERM=dumb on all child processes is a much bigger behavioral change than fixing sudo prompt corruption. Many tools branch on TERM (not just isatty) for things like less, line editing in shells, or color decisions independent of stdout.isatty. This will change output for every terminal command, not just interactive/sudo ones, and there's no test coverage showing what breaks. If the goal is just to strip escape codes from the captured output, that's already handled by stripAnsi - TERM=dumb risks changing what child programs actually do, which is a different (and larger) change than the title claims.

  3. Stripping all bare \r unconditionally could mangle any output that legitimately uses \r for multi-line progress reporting when displayed later - probably fine for this use case, but undocumented as a tradeoff.

  4. Three distinct fixes (ANSI stripping, SUDO_PROMPT, TERM=dumb) landed in one small diff with no tests. Each deserves independent justification and, ideally, a test showing before/after output for a representative sudo/interactive case.

Splitting this into the ANSI-stripping fix (clearly justified) and separately proposing the TERM/SUDO_PROMPT env changes (with rationale for why they don't break existing color-dependent workflows) would make this much easier to port.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written stale No activity after a maintainer request; queued for closing labels Aug 20, 2026
@codebuff-team

Copy link
Copy Markdown
Contributor

Marking this stale - there has been no activity here for 50 days. It will close in 7 days unless someone comments.

This is backlog upkeep, not a verdict on the pull request. A single comment keeps it open, and anything closed this way can be reopened.

@OmarAlaaeldein
OmarAlaaeldein force-pushed the fix/sudo-ansi-escape-tui-corruption branch from e54d409 to 00ee8c9 Compare August 23, 2026 20:09
@OmarAlaaeldein

Copy link
Copy Markdown
Author

Rebased onto current main and narrowed this PR to the ANSI-stripping fix only. It now uses the existing stripAnsi utility in the bounded output buffer, removes the TERM=dumb and SUDO_PROMPT changes, and does not strip bare carriage returns. Added a regression test for cursor-control sequences. Verified with: bun test sdk/src/tests/run-terminal-command.test.ts (15 passing).

@OmarAlaaeldein
OmarAlaaeldein force-pushed the fix/sudo-ansi-escape-tui-corruption branch from 00ee8c9 to 683e277 Compare August 23, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written stale No activity after a maintainer request; queued for closing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants